New default path for config file #4301

codebien · 2025-01-30T15:57:31Z

What?

It changes the default path for config file to .config/k6/config.json from the legacy loadimpact version.

It adds two changes:

Commands like k6 run that use the consolidated config get now a warning if the a config file is loaded from the legacy path.
k6 cloud login and k6 login commands attempt a migration from the old to the new path. They just do a copy of the file keeping the old file available, in this way old versions can still run using the old path and the user can roll back to the previous config file if something doesn't work as expected.

Why?

Check #2508

Related PR(s)/Issue(s)

Closes #2508

inancgumus

LGTM 👍 Feel free to skip my suggestions if they don't make sense.

internal/cmd/config.go

joanlopez

Overall it looks mostly fine, except for a few minor things I pointed out during the review.

However, I have the general doubt around when should we warn users and when should we perform the configuration migration. Isn't file disk configuration also used for other commands like run, archive or cloud run?

Why do we only consider it for k6 cloud login?
Do users normally don't store anything beyond cloud configuration? 🤔

joanlopez · 2025-02-04T11:39:35Z

internal/cmd/config.go

+}
+
+func readLegacyDiskConfig(gs *state.GlobalState) (Config, error) {
+	// CHeck if the legacy config exists in the supplied filesystem


Suggested change

// CHeck if the legacy config exists in the supplied filesystem

// Check if the legacy config exists in the supplied filesystem

joanlopez · 2025-02-04T11:54:06Z

internal/cmd/config.go

+		gs.Logger.Warn("The configuration file has been found on the old path. " +
+			"Please, run again `k6 cloud login` or `k6 login` commands to migrate to the new path. " +
+			"If you already migrated it manually, then remove the file from the old path.\n\n")


I think the warning is fine 👌🏻 but I'm not sure whether we should try to use the "new" config file path, if exists, as the default, or not. In such case, we may need to slightly adjust the message.

For instance, some of the scenarios that trigger those doubts in my mind are those in where the new configuration file path exists and is correct, while the old one also exists, but there are issues with them.

In all those cases, an error would be triggered, with no mention to migration. Perhaps too tricky.

I'm considering to reverse the flow, where we always check the new default before, and only eventually we check the old one. I didn't do that before because the code is a bit tricky in the current version because we are silencing the Not Found error. But I realized it's better to stretch a bit the code to fit it instead of yielding this cognitive load on the user.

joanlopez · 2025-02-04T12:00:27Z

internal/cmd/config.go

+		return Config{}, errext.WithExitCodeIfNone(err, exitcodes.InvalidConfig)
+	} else {
+		gs.Logger.Warn("The configuration file has been found on the old path. " +
+			"Please, run again `k6 cloud login` or `k6 login` commands to migrate to the new path. " +


Suggested change

"Please, run again `k6 cloud login` or `k6 login` commands to migrate to the new path. " +

"Please, run again `k6 cloud login` commands to migrate to the new path. " +

As k6 login has been deprecated, I think it's better to not mention it. Right? 🤔

If a user has a config file because it used k6 login influxdb but they don't have the Cloud they won't be able to migrate anymore. Do you think we should not support them?

Mmm.. not sure. Let's leave it for now, then. Although I guess that k6 login influxdb will eventually disappear.

joanlopez · 2025-02-04T12:03:03Z

internal/cmd/config.go

+		}
+
+		gs.Logger.Infof("Note, the configuration file has been migrated "+
+			"from old default path (%q) to the new version (%q).\n\n", legacyFpath, gs.Flags.ConfigFilePath)


Suggested change

"from old default path (%q) to the new version (%q).\n\n", legacyFpath, gs.Flags.ConfigFilePath)

"from the old default path (%q) to the new one (%q).\n\n", legacyFpath, gs.Flags.ConfigFilePath)

codebien · 2025-02-04T13:36:59Z

Hey @joanlopez, thanks for reviewing 👋

However, I have the general doubt around when should we warn users and when should we perform the configuration migration. Isn't file disk configuration also used for other commands like run, archive or cloud run?

I applied mostly what was suggested here #2508 (comment). It makes sense for me because it keeps the same operations' flows. k6 writes the config on k6 login operations, and it reads on all the others. I wouldn't break this principle, so it migrates (write) on k6 login and it warns (reads) on all the others.

In addition, re-reading Ned's comment, I realized I'm applying a reversed logic on the reading path, where we read before the legacy config and only after the new. But there isn't really a requirement for doing it, we can just directly read the new path and only if it doesn't exist then do an additional attempt on the legacy.

Does it make sense for you? The goal here is to provide the best UX so if you have any suggestion to improve it, I'm happy to evaluate them.

codebien · 2025-02-04T18:11:57Z

@inancgumus @joanlopez I have a doubt. During the migration, do you think should we delete the old directories (loadimpact/k6) or should we just leave them there to prevent any additional error?

inancgumus · 2025-02-04T19:49:48Z

@codebien

I have a doubt. During the migration, do you think should we delete the old directories (loadimpact/k6) or should we just leave them there to prevent any additional error?

If I understand the approach here correctly, it feels safer and more user-friendly to leave the old directory intact by default. Automatically deleting it can cause unintended data loss or break workflows if the user relies on that directory in ways we haven't anticipated. We can allow users to remove it themselves once they confirm everything works in the new location.

joanlopez · 2025-02-06T09:56:37Z

In addition, re-reading Ned's comment, I realized I'm applying a reversed logic on the reading path, where we read before the legacy config and only after the new. But there isn't really a requirement for doing it, we can just directly read the new path and only if it doesn't exist then do an additional attempt on the legacy.

Does it make sense for you? The goal here is to provide the best UX so if you have any suggestion to improve it, I'm happy to evaluate them.

Yeah, I think that likely makes more sense. So, we start using the new one, and only use the old one as a fallback, if existing.
And, only in such case, we may try to perform the migration.

@inancgumus @joanlopez I have a doubt. During the migration, do you think should we delete the old directories (loadimpact/k6) or should we just leave them there to prevent any additional error?

Here, I agree with @inancgumus, I'd play safe and just tell the user that we migrated the config, and so they're able to delete the old one manually

codebien · 2025-02-06T16:34:19Z

Hey @inancgumus @joanlopez I've done most of the discussed changes, take another look, please. 🙏
Make sure to test directly building the binary on your machines, as I'm not on OSX then we might discover bugs not visible to me on Linux.

inancgumus

LGTM 👍 It works on OS X 15.3, too.

joanlopez · 2025-02-10T09:53:50Z

LGTM 👍 It works on OS X 15.3, too.

Also tested on MacOS 15.3 and it also worked for me! 👍🏻

codebien self-assigned this Jan 30, 2025

codebien changed the base branch from master to config-file-tests January 30, 2025 15:57

codebien force-pushed the config-file-tests branch from a9e3047 to 4fe44f4 Compare January 30, 2025 16:03

codebien force-pushed the k6-config-path branch from d93279c to 7588059 Compare January 30, 2025 16:13

codebien mentioned this pull request Jan 31, 2025

Unit tests for config file operations #4298

Merged

codebien force-pushed the k6-config-path branch from 7588059 to 91710b9 Compare January 31, 2025 13:51

Base automatically changed from config-file-tests to master January 31, 2025 14:13

codebien force-pushed the k6-config-path branch from 91710b9 to 7e205bb Compare January 31, 2025 14:23

codebien added 5 commits January 31, 2025 15:59

Set a warning if the legacy config path is found

3d1a447

Changed the default config file path

0f91d52

Call the migration procedure from login commands

5d21cdb

Migration to the new default config path

2837203

Fix lint errors

43a3cd9

codebien force-pushed the k6-config-path branch from 7e205bb to 43a3cd9 Compare January 31, 2025 14:59

codebien marked this pull request as ready for review January 31, 2025 15:05

codebien requested a review from a team as a code owner January 31, 2025 15:05

codebien requested review from mstoykov, joanlopez, a team and inancgumus and removed request for a team and mstoykov January 31, 2025 15:05

inancgumus previously approved these changes Feb 3, 2025

View reviewed changes

internal/cmd/config.go Outdated Show resolved Hide resolved

internal/cmd/config.go Outdated Show resolved Hide resolved

internal/cmd/config.go Show resolved Hide resolved

codebien added 2 commits February 4, 2025 11:04

Do not use named return params

364b369

Reuse the utility function for the legacy path generation

70a2368

codebien dismissed inancgumus’s stale review via 70a2368 February 4, 2025 10:11

joanlopez reviewed Feb 4, 2025

View reviewed changes

Load the legacy only after attempted the default

89f454f

Adjust logs text

e07da87

codebien added 4 commits February 6, 2025 12:01

Use always the default path for the migration

3cb578d

Do not delete the old path

7096b8c

Added tests for load config operation

fb14f73

Docs nitpicks

9b2125c

codebien requested review from joanlopez and inancgumus February 6, 2025 16:32

inancgumus approved these changes Feb 6, 2025

View reviewed changes

inancgumus added this to the v0.57.0 milestone Feb 6, 2025

codebien modified the milestones: v0.57.0, v1.0.0-rc1 Feb 7, 2025

joanlopez approved these changes Feb 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New default path for config file #4301

New default path for config file #4301

codebien commented Jan 30, 2025 •

edited

Loading

inancgumus left a comment

joanlopez left a comment

joanlopez Feb 4, 2025

joanlopez Feb 4, 2025

joanlopez Feb 4, 2025

codebien Feb 4, 2025

joanlopez Feb 4, 2025

codebien Feb 4, 2025

joanlopez Feb 6, 2025

joanlopez Feb 4, 2025

codebien commented Feb 4, 2025

codebien commented Feb 4, 2025

inancgumus commented Feb 4, 2025

joanlopez commented Feb 6, 2025

codebien commented Feb 6, 2025

inancgumus left a comment

joanlopez commented Feb 10, 2025

	// CHeck if the legacy config exists in the supplied filesystem
	// Check if the legacy config exists in the supplied filesystem

	"Please, run again `k6 cloud login` or `k6 login` commands to migrate to the new path. " +
	"Please, run again `k6 cloud login` commands to migrate to the new path. " +

	"from old default path (%q) to the new version (%q).\n\n", legacyFpath, gs.Flags.ConfigFilePath)
	"from the old default path (%q) to the new one (%q).\n\n", legacyFpath, gs.Flags.ConfigFilePath)

New default path for config file #4301

Are you sure you want to change the base?

New default path for config file #4301

Conversation

codebien commented Jan 30, 2025 • edited Loading

What?

Why?

Related PR(s)/Issue(s)

inancgumus left a comment

Choose a reason for hiding this comment

joanlopez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codebien commented Feb 4, 2025

codebien commented Feb 4, 2025

inancgumus commented Feb 4, 2025

joanlopez commented Feb 6, 2025

codebien commented Feb 6, 2025

inancgumus left a comment

Choose a reason for hiding this comment

joanlopez commented Feb 10, 2025

codebien commented Jan 30, 2025 •

edited

Loading